Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add networking options. #277

Merged
merged 1 commit into from
Jul 27, 2023
Merged

Conversation

kurnevsky
Copy link
Contributor

Tested on my phone with yggdrasil - works fine :)

@kurnevsky
Copy link
Contributor Author

kurnevsky commented Jul 20, 2023

Hm, actually I might try to replicate more options from https://github.com/NixOS/nixpkgs/blob/nixos-23.05/nixos/modules/config/networking.nix , at least hosts and hostFiles seem useful.

@kurnevsky kurnevsky marked this pull request as draft July 20, 2023 15:31
@kurnevsky kurnevsky force-pushed the extra-hosts branch 3 times, most recently from f9aa6d5 to 3776ddc Compare July 20, 2023 15:39
@kurnevsky kurnevsky marked this pull request as ready for review July 20, 2023 15:40
@kurnevsky
Copy link
Contributor Author

ok, seems to work. I don't really know why nixpkgs defines 127.0.0.2 as localhost, so I didn't do that :)

@kurnevsky kurnevsky marked this pull request as draft July 20, 2023 16:41
@kurnevsky kurnevsky marked this pull request as ready for review July 20, 2023 16:44
@kurnevsky kurnevsky changed the title Add extraHosts option. Add networking options. Jul 20, 2023
@kurnevsky
Copy link
Contributor Author

127.0.0.2 as localhost

It's something from nss-myhostname, but we don't have systemd so we don't really need it.

@kurnevsky
Copy link
Contributor Author

Ping :)

@t184256
Copy link
Collaborator

t184256 commented Jul 26, 2023

Sorry for a slow response.

  1. If you're borrowing from nixpkgs, you need to add attribution. grep for 'under MIT license as well' for examples.
  2. Please bump the copyright year.
  3. I'm not a fan of the overdone nixos interface, but, I guess, we shouldn't deviate.
  4. But if we stick to it, I'd like a testcase exercising all these options + assertion path. You can write one or I can write one, but it might take some time for me to find time for that.

@kurnevsky
Copy link
Contributor Author

Sorry for a slow response.

No problem. Just wanted to make sure it's not forgotten.

I'm not a fan of the overdone nixos interface, but, I guess, we shouldn't deviate.

I personally use only networking.hosts, so I'm not really sure how other settings are useful. There are advantages of keeping it in sync with nixpkgs, but it's more complicated. I'm fine either way, I guess :)

But if we stick to it, I'd like a testcase exercising all these options + assertion path. You can write one or I can write one, but it might take some time for me to find time for that.

I can try to do that. It's me who wants these settings after all :)

@kurnevsky
Copy link
Contributor Author

But if we stick to it

So should we? :)

@t184256
Copy link
Collaborator

t184256 commented Jul 26, 2023 via email

@kurnevsky
Copy link
Contributor Author

ok, added a test. Wanted to use https://github.com/bats-core/bats-assert#assert_line but couldn't figure out how to add it in shebang - I guess some more complex changes are required for it.

@kurnevsky
Copy link
Contributor Author

btw, should I add myself to authors now? :)

@kurnevsky kurnevsky force-pushed the extra-hosts branch 2 times, most recently from 95c55d9 to fe60937 Compare July 26, 2023 18:54
Copy link
Collaborator

@t184256 t184256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test

good. hope grep is available in tests.

btw, should I add myself to authors now? :)

Feel free to. My idea was to let anyone edit it when they submit a PR and think it's substantial enough. BTW, if you know where to express this idea so that all contributors get it, I'm all ears.

Also, consider adding a changelog entry.

tests/on-device/config-flake-hosts.cfg.nix Outdated Show resolved Hide resolved
@kurnevsky
Copy link
Contributor Author

good. hope grep is available in tests.

It is.

BTW, if you know where to express this idea so that all contributors get it, I'm all ears.

Well, readme with or Contributing.md with some basic steps should be enough :)

Also, consider adding a changelog entry.

Done.

@kurnevsky
Copy link
Contributor Author

kurnevsky commented Jul 26, 2023

It seems formatting is not correct. How do I format it properly?

Probably nixpkgs-fmt... There are a lot of different nix formatters :)

Copy link
Collaborator

@t184256 t184256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one typo, but otherwise it looks good to me.

Please fix that one, squash into a commit named like modules/environment/networking: implement /etc/hosts options and let's merge.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@t184256 t184256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you for your contribution, patience and tests!

@t184256 t184256 merged commit b324ef2 into nix-community:master Jul 27, 2023
4 checks passed
@kurnevsky kurnevsky deleted the extra-hosts branch July 27, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants